-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[UIAM] Cloud API key authentication metadata and validations #129227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[UIAM] Cloud API key authentication metadata and validations #129227
Conversation
…ud-api-key-authentication
…ud-api-key-authentication # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java
…hentication-metadata-and-validation # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java
…hentication-metadata-and-validation # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/AuthenticationField.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/AuthenticationSerializationTests.java # x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
…hentication-metadata-and-validation
|
Pinging @elastic/es-security (Team:Security) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same on this PR: looks good, but I want to take a bit of time in the morning to fully complete the review. Don't foresee big change requests, nice work 👍
A couple suggestions.
| } | ||
| checkConsistencyForApiKeyAuthenticatingSubject("API key"); | ||
| final String prefixMessage = authenticatingRealm.isCloudApiKeyRealm() ? "Cloud API key" : "API key"; | ||
| checkConsistencyForApiKeyAuthenticatingSubject(prefixMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer a separate method checkConsistencyForCloudApiKeyAuthenticatingSubject for cloud API keys here -- cloud API keys are fundamentally different from stack API keys. For example, we don't expect role descriptors in cloud API key metadata. We currently don't check for the presence of role descriptors for stack API keys either but could easily do that in the future. Similarly I think it's a good consistency check that cloud API keys don't have role descriptors in metadata.
I'd also add a check to checkConsistencyForApiKeyAuthenticatingSubject that internal is not set since that doesn't make sense for stack API keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ Agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed the changes which add dedicated consistency check for cloud API keys and additionally made run-as unsupported for cloud API keys. Currently, there is no need to support them. We can always enable it in the future.
| public static final String API_KEY_CREATOR_REALM_TYPE = "_security_api_key_creator_realm_type"; | ||
| public static final String API_KEY_ID_KEY = "_security_api_key_id"; | ||
| public static final String API_KEY_NAME_KEY = "_security_api_key_name"; | ||
| public static final String API_KEY_MANAGED_BY_KEY = "_security_api_key_managed_by"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this metadata field -- we can always infer the value based on the subject type. Less is more in this case, IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. FWIW I initially started by inferring it, then realized we planned to have it for all authentication types. Since we are not requring it for all, just API keys (at the moment), I'll change this and remove it.
...core/src/test/java/org/elasticsearch/xpack/core/security/authc/AuthenticationTestHelper.java
Show resolved
Hide resolved
...security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java
Outdated
Show resolved
Hide resolved
...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java
Outdated
Show resolved
Hide resolved
...core/src/test/java/org/elasticsearch/xpack/core/security/authc/AuthenticationTestHelper.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed on Zoom: slight leaning towards not storing api key ID in metadata since it enforces that the API key ID is the principal for cloud API keys and achieves consistency by definition instead of requiring explicit consistency checks (API key ID == principal for cloud API keys).
Looks good otherwise!
...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java
Outdated
Show resolved
Hide resolved
...ugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/AuthenticationTests.java
Outdated
Show resolved
Hide resolved
…hentication-metadata-and-validation
…hentication-metadata-and-validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
…#129227) A followup to elastic#128440, which introduces a new `managed_by` field (`<1>`) that will be returned in the response of the Authenticate API. Besides `managed_by` field, it also captures additional `internal` field (`<2>`) for cloud API key authentication and exposes it as part of the `api_key` fields. ```json { "username": "omSAd5YBK3gZiBcD-GvX", "roles": [ "viewer" ], "metadata": { ... }, "enabled": true, "authentication_realm": { "name": "_cloud_api_key", "type": "_cloud_api_key" }, "lookup_realm": { "name": "_cloud_api_key", "type": "_cloud_api_key" }, "authentication_type": "api_key", "api_key": { "id": "omSAd5YBK3gZiBcD-GvX", "name": "my cloud API key", "managed_by": "cloud", <1> "internal": false <2> } } ``` - Additionally it implements the `Authentication#canAccessResourcesOf` for the cloud API keys. Ownership check allows access only to the same cloud API key. - And lastly, adds a consistency check for cloud API keys in `Authentication#checkConsistencyForApiKeyAuthenticationType`.
…#129227) A followup to elastic#128440, which introduces a new `managed_by` field (`<1>`) that will be returned in the response of the Authenticate API. Besides `managed_by` field, it also captures additional `internal` field (`<2>`) for cloud API key authentication and exposes it as part of the `api_key` fields. ```json { "username": "omSAd5YBK3gZiBcD-GvX", "roles": [ "viewer" ], "metadata": { ... }, "enabled": true, "authentication_realm": { "name": "_cloud_api_key", "type": "_cloud_api_key" }, "lookup_realm": { "name": "_cloud_api_key", "type": "_cloud_api_key" }, "authentication_type": "api_key", "api_key": { "id": "omSAd5YBK3gZiBcD-GvX", "name": "my cloud API key", "managed_by": "cloud", <1> "internal": false <2> } } ``` - Additionally it implements the `Authentication#canAccessResourcesOf` for the cloud API keys. Ownership check allows access only to the same cloud API key. - And lastly, adds a consistency check for cloud API keys in `Authentication#checkConsistencyForApiKeyAuthenticationType`.
…#129227) A followup to elastic#128440, which introduces a new `managed_by` field (`<1>`) that will be returned in the response of the Authenticate API. Besides `managed_by` field, it also captures additional `internal` field (`<2>`) for cloud API key authentication and exposes it as part of the `api_key` fields. ```json { "username": "omSAd5YBK3gZiBcD-GvX", "roles": [ "viewer" ], "metadata": { ... }, "enabled": true, "authentication_realm": { "name": "_cloud_api_key", "type": "_cloud_api_key" }, "lookup_realm": { "name": "_cloud_api_key", "type": "_cloud_api_key" }, "authentication_type": "api_key", "api_key": { "id": "omSAd5YBK3gZiBcD-GvX", "name": "my cloud API key", "managed_by": "cloud", <1> "internal": false <2> } } ``` - Additionally it implements the `Authentication#canAccessResourcesOf` for the cloud API keys. Ownership check allows access only to the same cloud API key. - And lastly, adds a consistency check for cloud API keys in `Authentication#checkConsistencyForApiKeyAuthenticationType`.
A followup to #128440, which introduces a new
managed_byfield (<1>) that will be returned in the response of the Authenticate API.Besides
managed_byfield, it also captures additionalinternalfield (<2>) for cloud API key authentication and exposes it as part of theapi_keyfields.{ "username": "omSAd5YBK3gZiBcD-GvX", "roles": [ "viewer" ], "metadata": { ... }, "enabled": true, "authentication_realm": { "name": "_cloud_api_key", "type": "_cloud_api_key" }, "lookup_realm": { "name": "_cloud_api_key", "type": "_cloud_api_key" }, "authentication_type": "api_key", "api_key": { "id": "omSAd5YBK3gZiBcD-GvX", "name": "my cloud API key", "managed_by": "cloud", <1> "internal": false <2> } }Additionally it implements the
Authentication#canAccessResourcesOffor the cloud API keys. Ownership check allows access only to the same cloud API key.And lastly, adds a consistency check for cloud API keys in
Authentication#checkConsistencyForApiKeyAuthenticationType.